-
-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: pre-electra support from attestation pool #6998
Conversation
c6e1cca
to
c9394c6
Compare
af660d5
to
97b089a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General approach looks good to me, haven't reviewed in detail
lgtm |
97b089a
to
99a85a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more comments
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
f83cbf3
to
72b8074
Compare
Co-authored-by: Nico Flaig <[email protected]>
Co-authored-by: Nico Flaig <[email protected]>
…ol.ts Co-authored-by: Nico Flaig <[email protected]>
* fix: getSeenAttDataKey apis * chore: use ForkName instead of ForkSeq
f0e83b6
to
4c81a0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just a few remarks
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/test/perf/chain/opPools/aggregatedAttestationPool.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Nico Flaig <[email protected]>
…ol.ts Co-authored-by: Nico Flaig <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## electra-fork-rebasejul30 #6998 +/- ##
===========================================================
Coverage ? 49.35%
===========================================================
Files ? 589
Lines ? 39186
Branches ? 2238
===========================================================
Hits ? 19342
Misses ? 19803
Partials ? 41 |
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts
Outdated
Show resolved
Hide resolved
…ol.ts Co-authored-by: Nico Flaig <[email protected]>
looks like last round of review to be addressed, then we can merge this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Initial commit * Update packages/beacon-node/src/chain/chain.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/api/impl/validator/index.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Add unit test for attestation pool * fix: getSeenAttDataKey apis (#7009) * fix: getSeenAttDataKey apis * chore: use ForkName instead of ForkSeq * Update packages/beacon-node/src/util/sszBytes.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Update error message * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Move determining post-electra fork out of loops --------- Co-authored-by: Nico Flaig <[email protected]> Co-authored-by: twoeths <[email protected]>
* Initial commit * Update packages/beacon-node/src/chain/chain.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/api/impl/validator/index.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Add unit test for attestation pool * fix: getSeenAttDataKey apis (#7009) * fix: getSeenAttDataKey apis * chore: use ForkName instead of ForkSeq * Update packages/beacon-node/src/util/sszBytes.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Update error message * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Move determining post-electra fork out of loops --------- Co-authored-by: Nico Flaig <[email protected]> Co-authored-by: twoeths <[email protected]>
* Initial commit * Update packages/beacon-node/src/chain/chain.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/api/impl/validator/index.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Add unit test for attestation pool * fix: getSeenAttDataKey apis (#7009) * fix: getSeenAttDataKey apis * chore: use ForkName instead of ForkSeq * Update packages/beacon-node/src/util/sszBytes.ts Co-authored-by: Nico Flaig <[email protected]> * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Update error message * Update packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Co-authored-by: Nico Flaig <[email protected]> * address comment * Move determining post-electra fork out of loops --------- Co-authored-by: Nico Flaig <[email protected]> Co-authored-by: twoeths <[email protected]>
🎉 This PR is included in v1.22.0 🎉 |
aggregateByIndexByRootBySlot
is modified to allow null committee index to only handle pre-electra attestations. null committee index is not allowed post-electra attestationsbeacon_attestation
in gossip queue is indexed by attestation data instead of attestation data + committee bits.